*: get tiflash http(s) port from tiflash status port instead of etcd #30901

lidezhu · 2021-12-21T06:43:48Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary: There are two system tables about tiflash in tidb that need to read data from tiflash http(s) port. Previously tidb get http(s) port from etcd which was reported by tiflash. But from v5.3, we can get tiflash config from tiflash status port and so we can parse the http(s) port from the config instead of etcd.

What is changed and how it works?

The system tables' behavior remains the same as in #18092 and #19714.
The only difference is that we get http(s) port from the config fetched from tiflash status port instead of etcd.
The detailed steps are as following:

get tiflash_proxy_status_port for every tiflash instance;
get tiflash config from the url: {tiflash_host}:{tiflash_proxy_status_port}/config;
get tiflash http_port or https_port from the config items engine-store.http_port or engine-store.https_port;

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
deploy a tidb cluster and check we can select from the two tiflash system tables tiflash_segments and tiflash_tables as before.

Release note

None

ti-chi-bot · 2021-12-21T06:43:49Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

fzhedu
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lidezhu · 2021-12-21T06:53:07Z

/cc @crazycs520 @windtalker PTAL

sre-bot · 2021-12-21T09:54:19Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/3f78a2807fa7af96097359d89a6d1d0fbb638dbc

executor/infoschema_reader.go

lidezhu · 2021-12-22T05:29:32Z

/run-unit-test

ti-chi-bot · 2021-12-28T02:52:13Z

@CalvinNeo: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

executor/infoschema_reader.go

windtalker

LGTM

lidezhu · 2021-12-28T06:06:08Z

/merge

ti-chi-bot · 2021-12-28T06:06:11Z

@lidezhu: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

fzhedu · 2021-12-28T07:13:47Z

executor/infoschema_reader.go

+		}
+		info.ResolveLoopBackAddr()
+		if len(tiflashInstances) > 0 && !tiflashInstances.Exist(info.Address) {
+			continue


append a warning?

tiflashInstances is just the where clause in the select list.
For example, if the sql is select * from tiflash_tables, then tiflashInstances is empty and it means we need to select the info for all tiflash nodes in the cluster.
If the sql is select * from tiflash_tables where tiflash_instance='127.0.0.1:9000', then tiflashInstances contains the element 127.0.0.1:9000 and we need filter out other tiflash nodes.

fzhedu · 2021-12-28T07:15:32Z

executor/infoschema_reader.go

+		if err != nil {
+			sctx.GetSessionVars().StmtCtx.AppendWarning(err)
+			continue
+		}
+		if resp.StatusCode != http.StatusOK {
+			return errors.Errorf("request %s failed: %s", configURL, resp.Status)
+		}


should append warning or return error first?

If tiflash doesn't return response, it means the tiflash node is down or is network partitioned, we can safely ignore the tiflash node and just append warning.
But if tiflash can return response, but the status code is not StatusOK, then something bad unknown must happen and an error is perfered to remind this case.

fzhedu · 2021-12-28T07:19:23Z

what if tiflash config has errors of format? like 127...0.0:2222222

lidezhu · 2021-12-28T07:48:59Z

what if tiflash config has errors of format? like 127...0.0:2222222

The config read from tiflash status port is already checked by tiflash instance. If the http_port is not a valid value, then tiflash cannot start.

fzhedu

LGTM

fzhedu · 2021-12-28T07:54:37Z

/merge

ti-chi-bot · 2021-12-28T07:54:40Z

This pull request has been accepted and is ready to merge.

Commit hash: cfbad4e

ti-chi-bot · 2021-12-28T08:03:59Z

@lidezhu: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

lidezhu · 2021-12-28T08:16:17Z

/rebuild

lidezhu · 2021-12-28T08:21:30Z

/merge

ti-chi-bot · 2021-12-28T08:21:33Z

@lidezhu: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

get tiflash http(s) port from config instead of etcd

1cce89c

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 21, 2021

lidezhu changed the title *: get tiflash http(s) port from config instead of etcd *: get tiflash http(s) port from tiflash status port instead of etcd Dec 21, 2021

ti-chi-bot requested review from crazycs520 and windtalker December 21, 2021 06:53

lidezhu added 3 commits December 21, 2021 14:57

fix check dev

51e6f5b

fix wrong fmt parameters

04ba0c0

fix unit test

80a0998

CalvinNeo reviewed Dec 22, 2021

View reviewed changes

executor/infoschema_reader.go Outdated Show resolved Hide resolved

CalvinNeo reviewed Dec 22, 2021

View reviewed changes

executor/infoschema_reader.go Show resolved Hide resolved

use Get instead of create a explicit request

1837f02

CalvinNeo approved these changes Dec 28, 2021

View reviewed changes

windtalker reviewed Dec 28, 2021

View reviewed changes

executor/infoschema_reader.go Show resolved Hide resolved

windtalker approved these changes Dec 28, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 28, 2021

Merge branch 'master' into improve-tiflash-system-tables

fabb25f

fzhedu reviewed Dec 28, 2021

View reviewed changes

fzhedu approved these changes Dec 28, 2021

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 28, 2021

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 28, 2021

Merge branch 'master' into improve-tiflash-system-tables

cfbad4e

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 28, 2021

Merge branch 'master' into improve-tiflash-system-tables

3f78a28

ti-chi-bot merged commit f35e84c into pingcap:master Dec 28, 2021

lidezhu deleted the improve-tiflash-system-tables branch December 28, 2021 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: get tiflash http(s) port from tiflash status port instead of etcd #30901

*: get tiflash http(s) port from tiflash status port instead of etcd #30901

lidezhu commented Dec 21, 2021 •

edited

Loading

ti-chi-bot commented Dec 21, 2021 •

edited

Loading

lidezhu commented Dec 21, 2021 •

edited

Loading

sre-bot commented Dec 21, 2021 •

edited

Loading

lidezhu commented Dec 22, 2021

ti-chi-bot commented Dec 28, 2021

windtalker left a comment

lidezhu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

fzhedu Dec 28, 2021

lidezhu Dec 28, 2021 •

edited

Loading

fzhedu Dec 28, 2021

lidezhu Dec 28, 2021 •

edited

Loading

fzhedu commented Dec 28, 2021

lidezhu commented Dec 28, 2021

fzhedu left a comment

fzhedu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

lidezhu commented Dec 28, 2021

lidezhu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

*: get tiflash http(s) port from tiflash status port instead of etcd #30901

*: get tiflash http(s) port from tiflash status port instead of etcd #30901

Conversation

lidezhu commented Dec 21, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 21, 2021 • edited Loading

lidezhu commented Dec 21, 2021 • edited Loading

sre-bot commented Dec 21, 2021 • edited Loading

lidezhu commented Dec 22, 2021

ti-chi-bot commented Dec 28, 2021

windtalker left a comment

Choose a reason for hiding this comment

lidezhu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

fzhedu Dec 28, 2021

Choose a reason for hiding this comment

lidezhu Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

fzhedu Dec 28, 2021

Choose a reason for hiding this comment

lidezhu Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

fzhedu commented Dec 28, 2021

lidezhu commented Dec 28, 2021

fzhedu left a comment

Choose a reason for hiding this comment

fzhedu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

lidezhu commented Dec 28, 2021

lidezhu commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

lidezhu commented Dec 21, 2021 •

edited

Loading

ti-chi-bot commented Dec 21, 2021 •

edited

Loading

lidezhu commented Dec 21, 2021 •

edited

Loading

sre-bot commented Dec 21, 2021 •

edited

Loading

lidezhu Dec 28, 2021 •

edited

Loading

lidezhu Dec 28, 2021 •

edited

Loading